remove _from_reload from module name to avoid confusion#6033
Conversation
viambot
added
the
safe to test
dgottlieb
left a comment
dgottlieb
left a comment
There was a problem hiding this comment.
I'm assuming the consequences of this are understood -- but let me know if I should be looking closer.
| // (or "<orgID>:<moduleName>" if no namespace is set) into a name suitable for | ||
| // adding to a robot config, i.e. "<namespace>_<moduleName>" or "<orgID>_<moduleName>". | ||
| // TODO(APP-4019): remove this logic after registry modules can have local ExecPath. | ||
| func localizeModuleID(moduleID string) string { |
There was a problem hiding this comment.
I'm not entirely privy to the callers of this and how this affects the robot config we generate. Specifically if removing _from_reload results in module name collisions we wouldn't expect before. Do you need any additional vetting from me that this is kosher?
There was a problem hiding this comment.
I see your concern, but i think its fine. The app, rdk, and cli are resilient on changes in the name field of the JSON. The uniqueness and searching is on the module_id field and not the name field for modules https://github.com/viamrobotics/rdk/blob/main/cli/module_reload.go#L413. The behavior for modules is different than other areas of the config that treat name as needing to be unique. It is also currently allowed that you can change name field in app and reload will still find the module, so I'm not concerned about this. Also the RDK allows multiple modules with the same name, but not with the same moduleID (I just tested to reaffirm my understanding).
4 participants
I've heard that having the module name be generated with "from_reload" is confusing after hot reloading is turned off. The machine's logs will still reference the module name string after the module has been switched to a registry semver version, making it seem like the module is still running the reloaded version.